Skip to content

refactor: unify APK building and installing process#1055

Closed
krizzu wants to merge 3 commits into
react-native-community:mainfrom
krizzu:buildApkFixes
Closed

refactor: unify APK building and installing process#1055
krizzu wants to merge 3 commits into
react-native-community:mainfrom
krizzu:buildApkFixes

Conversation

@krizzu

@krizzu krizzu commented Mar 17, 2020

Copy link
Copy Markdown
Member

Summary:

Hey!

While working on #1038, I've noticed that there are two different ways to build and install APK: one with --deviceId provided and one without. This PR unifies that process.

What has changed

  • Moved all methods related to APK build and install to buildAndInstallApk.ts (including utility functions).

  • Removed old and unused code.

  • Exported two new functions :

    • buildApk - builds APK using provided variants.
    • installApk - checks if APK has been build for specified variant. If it is, use adb to install. Otherwise use gradle's install* task to build and install.

To Do:

  • cleanup
  • tests

Test Plan:

Green CI.

@thymikee

Copy link
Copy Markdown
Member

How's it going, do you need any help here?

@krizzu

krizzu commented Mar 19, 2020

Copy link
Copy Markdown
Member Author

Hey, working on Jest tests and manual testing too - should be here soonish

Comment thread packages/platform-android/src/commands/runAndroid/index.ts
@krizzu krizzu changed the title [WIP] refactor: unify APK building and installing process refactor: unify APK building and installing process Mar 22, 2020
@krizzu

krizzu commented Mar 23, 2020

Copy link
Copy Markdown
Member Author

This one is ready for some reviews, thanks!

Comment thread packages/platform-android/src/commands/runAndroid/buildAndInstallApk.ts Outdated
@krizzu krizzu force-pushed the buildApkFixes branch 2 times, most recently from 648389a to acf762a Compare April 17, 2020 17:00
@krizzu

krizzu commented Apr 20, 2020

Copy link
Copy Markdown
Member Author

@thymikee @Esemesek
Any idea why those doctor e2e tests fails?

@thymikee

Copy link
Copy Markdown
Member

Azure fails with "EPERM: operation not permitted, mkdir 'D:'" when we try to create a temporary directory for testing, maybe something changed there? Sounds like Azure changing something, would be good to verify this in other PRs

@Esemesek

Copy link
Copy Markdown
Member

I think this test fails randomly. We consider Windows failures as non-blocking since most of them are just flakiness in the Azure environment.

@grabbou

grabbou commented May 26, 2020

Copy link
Copy Markdown
Member

Do you feel like giving this one a go @thymikee @Esemesek before shipping? Since it's ready and failures are non-related, I'd love to have a final approval from your side.

@grabbou grabbou self-assigned this Sep 17, 2020
@grabbou

grabbou commented Sep 17, 2020

Copy link
Copy Markdown
Member

Since this code is ready to go and this is our second attempt and rewriting this relatively major part of run-android, I am going to take it over and do the last round of testing myself.

I am hoping we can ship it next week, it should resolve some really annoying issues.

@driiftkiing

Copy link
Copy Markdown

@thymikee: i have rebased the branch of this pr with the master into this branch https://github.com/tokyodrift1993/react-native-community-cli/tree/feature/buildApkFixes

@thymikee

Copy link
Copy Markdown
Member

@driiftkiing can you send a PR amending this one? :)

@driiftkiing

Copy link
Copy Markdown

@driiftkiing can you send a PR amending this one? :)

just creating a new one? or something else?

@thymikee

Copy link
Copy Markdown
Member

Yup, a new one. Unless @krizzu is still interested in pursuing this, but let's assume he's not and make sure to attribute him for the work done when merging

@krizzu

krizzu commented Jun 30, 2021

Copy link
Copy Markdown
Member Author

@driiftkiing @thymikee
Been a while 🤖
I just rebased the code, tests are passing. I'm yet to test this, making sure that it works properly.

@krizzu

krizzu commented Jun 30, 2021

Copy link
Copy Markdown
Member Author

Ran some tests on custom android project, trying different configuration of build variants (with and without flavors) and it works.
Would love to have someone else do the sanity check as well 🙏

@grabbou

grabbou commented Jul 6, 2021

Copy link
Copy Markdown
Member

Thanks @krizzu, I am going to do another pass at it this week and ship it!

@driiftkiing

Copy link
Copy Markdown

I will do also some tests, if i got the time

@thymikee

Copy link
Copy Markdown
Member

Thanks for the review @andriytsaryov. @grabbou I guess we can finally go ahead and merge it

@grabbou

grabbou commented Jul 15, 2021

Copy link
Copy Markdown
Member

Yes, that's on my todo list for tomorrow. When ready, I will create a new release.

await buildApk(args, gradlew, androidProject);
await installApk(args, gradlew, adbPath, devices, androidProject);

try {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this code is already moved out to buildApk and installApk respectively, e.g. args.port and createInstallError. I am going to go ahead and remove it.

@pas256

pas256 commented Feb 4, 2022

Copy link
Copy Markdown

I'd love to be able to run

npx react-native run-android --variant=developmentDebug --deviceId 931X1X9JW

and have it work. This PR seems to solve that. Anything holding this up getting merged in?

@grabbou

grabbou commented Feb 4, 2022

Copy link
Copy Markdown
Member

No, @pas256, this PR will be pulled in soon, once we merge a PR removing link and unlink.

@driiftkiing

driiftkiing commented Feb 7, 2022

Copy link
Copy Markdown

if anyone is interested to use this PR until it is merged:


command to run:

yarn react-native run-android --variant=developmentDebug

@adamTrz adamTrz mentioned this pull request Nov 9, 2022
5 tasks
@github-actions

Copy link
Copy Markdown

There hasn't been any activity on this pull request in the past 3 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days.

@github-actions github-actions Bot added the stale label Nov 28, 2022
@TMisiukiewicz

Copy link
Copy Markdown
Collaborator

#1739 closes this one

@github-actions github-actions Bot removed the stale label Nov 29, 2022
@adamTrz adamTrz closed this in #1739 Dec 6, 2022
@krizzu krizzu deleted the buildApkFixes branch December 6, 2022 15:10
@krizzu

krizzu commented Dec 6, 2022

Copy link
Copy Markdown
Member Author

🫡

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants